refactor: delay addition of issuer to auctioneer until a good time #8398

Chris-Hibbert · 2023-09-27T23:23:25Z

closes: #8347

Description

When adding a new asset type, delay until the auction is quiescent, so as not to trigger #8296.

Security Considerations

Don't break the chain.

Scaling Considerations

Not an issue here.

Documentation Considerations

Not applicable.

Testing Considerations

I added bootstrapTests that wait until a propitious or hazardous time to add the collateral, and verify that addAssetsToVault waits for the auction to quiesce, and succeeds in adding a new collateral type.

Upgrade Considerations

This applies to adding asset types, not upgrading contracts.

Chris-Hibbert · 2023-09-28T00:31:35Z

@turadg

I added a test file. ~~I haven't yet found the auction in order to query for the current auction schedule.~~ I ~~want to~~ test running addAssetsToVaults at these times:

after an auction has started
more than 20 minutes before the next auction starts
less than 20 minutes before the next auction starts

I think those would cover all the interesting cases, though it may be that i'm not thinking of enough edge conditions, so just before an auction starts, or just before it completes might give results I don't expect.

turadg

first pass

packages/inter-protocol/src/proposals/addAssetToVault.js

turadg · 2023-10-05T00:40:38Z

packages/inter-protocol/src/proposals/addAssetToVault.js

+  const addBrandThenResolve = ToFarFunction('addBrandThenResolve', async () => {
+    await E(auctioneerCreator).addBrand(interchainIssuer, keyword);
+    finishPromiseKit.resolve(true);
+    return true;


I don't see this value read. I think this function is meant to be void.

turadg · 2023-10-05T00:42:13Z

packages/inter-protocol/src/proposals/addAssetToVault.js

+  const finishPromiseKit = makePromiseKit();
+  const addBrandThenResolve = ToFarFunction('addBrandThenResolve', async () => {
+    await E(auctioneerCreator).addBrand(interchainIssuer, keyword);
+    finishPromiseKit.resolve(true);


I don't see this value read. I think it's meant to be:

Suggested change

finishPromiseKit.resolve(true);

finishPromiseKit.resolve(undefined);

turadg · 2023-10-05T00:46:59Z

packages/boot/test/bootstrapTests/test-addAssets.ts

+  makeLiquidationTestContext,
+} from './liquidation.ts';
+
+const test = anyTest as TestFn<LiquidationTestContext>;


I copied this from a nearby test. I don't even know what it means. Care to explain? What's it do, and why is it "grand"?

grand that you're using TS syntax ;-)

It means: "define test to be the value of anyTest with the type TestFn with its Context type parameter set to LiquidationTestContext". TestFn ensures that any test methods and descendants (like t) are typed The default t.context is unknown so the parameter slot fills that in with the actual context being assigned in line 16.

Though I see on line 30 that you're adding proposal to the context. To not get a type error, include it here.

Suggested change

const test = anyTest as TestFn<LiquidationTestContext>;

const test = anyTest as TestFn<LiquidationTestContext & { proposal: Awaited<ReturnType<typeof buildProposal>> }>;

Though seeing further down, I don't think the raw proposal should go into the context because it's really more of a template. See request below for providing a helper function instead.

done, though the declarations are a little more involved than that.

turadg · 2023-10-05T00:48:14Z

packages/boot/test/bootstrapTests/test-addAssets.ts

+  await advanceTimeTo(afterEndTime);
+
+  const schedulesAfter = await EV(auctioneerKit.creatorFacet).getSchedule();
+  // brand equality is broken


how and why?

TimeMath.compareAbs() complains that the brands don't match. I dunno why, but I suspect it's a result of whatever forces us to use EV here instead of E. How should I phrase that?

I'm surprised object identity of brands isn't preserved here. I thought maybe you were getting the brands from 2 different places - one from the kernel and one from vstorage - but not so; both are coming from calls to the creatorFacet by way of the kernel.

EV used to preserve object identity in such circumstances, I believe.

@gibson042 @FUDCo was there a regression in #8373 ? Or is my mental model just off?

Well, it's partially that your mental model is a bit off but there was a change that is part of the story here. You cannot directly compare object identity on the stuff you get back from EV, though reason is not actually EV itself but kmarshal, which previously had internal state that we've now gotten rid of because it was fundamentally incompatible with importation into different compartments. To address this there are now a handful of matchXXX functions in the same supports module with EV. To compare two object references (such as to brands) for equality, the one you want is matchRef.

(Though to be clear, there are some important limitations of EV that people seem to be oblivious to that they need to be more broadly educated about, but I don't think they're involved in this particular case.)

You cannot directly compare object identity on the stuff you get back from EV

noted. thanks for confirming.

I updated the comment to clarify the reason it can't use TimeMath.compareAbs().

dckc · 2023-10-05T18:12:10Z

Does this somehow subsume the oracle constraint from #8347 ?

... it's crucial that the oracle be started before the auction ...

Chris-Hibbert · 2023-10-05T18:17:49Z

Does this somehow subsume the oracle constraint from #8347 ?

... it's crucial that the oracle be started before the auction ...

It does not. If we're worried about that, we should add a check somewhere that an oracle is available.

dckc · 2023-10-05T18:26:23Z

Does this somehow subsume the oracle constraint from #8347 ?

... it's crucial that the oracle be started before the auction ...

It does not. If we're worried about that, we should add a check somewhere that an oracle is available.

My understanding of #8347 is that we are worried about that. Please do add a check that the oracle is available; for example, ask for (and await) a quote.

p.s. later discussion concluded that yes, this PR does subsume the oracle constraint check.

turadg

Logic of proposal and test look correct to me.

Please do the requested refactorings before merge.

turadg · 2023-10-05T18:15:05Z

packages/boot/test/bootstrapTests/test-addAssets.ts

+  makeLiquidationTestContext,
+} from './liquidation.ts';
+
+const test = anyTest as TestFn<LiquidationTestContext>;


grand that you're using TS syntax ;-)

It means: "define test to be the value of anyTest with the type TestFn with its Context type parameter set to LiquidationTestContext". TestFn ensures that any test methods and descendants (like t) are typed The default t.context is unknown so the parameter slot fills that in with the actual context being assigned in line 16.

Though I see on line 30 that you're adding proposal to the context. To not get a type error, include it here.

Suggested change

const test = anyTest as TestFn<LiquidationTestContext>;

const test = anyTest as TestFn<LiquidationTestContext & { proposal: Awaited<ReturnType<typeof buildProposal>> }>;

turadg · 2023-10-05T18:38:06Z

packages/boot/test/bootstrapTests/test-addAssets.ts

+});
+
+test.after.always(t => {
+  // This will fail if a subset of tests are run. It detects that three


Why do we need this check? Is each test not independent?

Each test now ends by verifying that readLatest(${auctioneerPath}.book1) finds something. What's actually happening is that the tests will sequentially add book1, book2, and book3. In order to allow for parallelism, since they take so long, I didn't make them sequential, so I don't know what order they run in.

I could have each test check all three potentially-present nodes and verify that one has a matching name. I thought it more straightforward to verify in the .after that 3 had been added. Please let me know if you think there's a more legible approach.

Actually, now that I moved building the proposal into .before, the total time was 38s. When I modified the tests to .serial that grew to 40s, which is probably not significant.

not significant. also probably a smaller difference in CI with fewer cores.

packages/boot/test/bootstrapTests/test-addAssets.ts

turadg · 2023-10-05T18:41:04Z

packages/boot/test/bootstrapTests/test-addAssets.ts

+  makeLiquidationTestContext,
+} from './liquidation.ts';
+
+const test = anyTest as TestFn<LiquidationTestContext>;


Though seeing further down, I don't think the raw proposal should go into the context because it's really more of a template. See request below for providing a helper function instead.

packages/boot/test/bootstrapTests/test-addAssets.ts

Chris-Hibbert self-assigned this Sep 27, 2023

Chris-Hibbert force-pushed the 8347-timing branch 3 times, most recently from 178cc76 to 6e90832 Compare October 4, 2023 22:36

Chris-Hibbert requested a review from turadg October 4, 2023 22:56

Chris-Hibbert force-pushed the 8347-timing branch from 6e90832 to 2a8fc5d Compare October 4, 2023 23:13

turadg reviewed Oct 5, 2023

View reviewed changes

turadg approved these changes Oct 5, 2023

View reviewed changes

dckc mentioned this pull request Oct 5, 2023

auction timing 0xpatrickdev/agoric-vault-collateral-proposal#21

Closed

Chris-Hibbert added auction automerge:rebase Automatically rebase updates, then merge labels Oct 5, 2023

Chris-Hibbert added 5 commits October 6, 2023 17:28

refactor: delay addition of issuer to auctioneer until a good time

412cd9e

test: test for adding assets at various times

d589ef3

chore: clean-ups and clarifications

db080ce

test: refactor test context setup to share propsoal mutation.

539e3dd

chore: update comment on object comparison breakage

6d52bcb

Chris-Hibbert force-pushed the 8347-timing branch from 2ea7c35 to 6d52bcb Compare October 6, 2023 17:28

mergify bot merged commit 61383f9 into master Oct 6, 2023
71 checks passed

mergify bot deleted the 8347-timing branch October 6, 2023 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: delay addition of issuer to auctioneer until a good time #8398

refactor: delay addition of issuer to auctioneer until a good time #8398

Chris-Hibbert commented Sep 27, 2023 •

edited

Loading

Chris-Hibbert commented Sep 28, 2023 •

edited

Loading

turadg left a comment

turadg Oct 5, 2023

Chris-Hibbert Oct 5, 2023

turadg Oct 5, 2023

Chris-Hibbert Oct 5, 2023

turadg Oct 5, 2023

Chris-Hibbert Oct 5, 2023

turadg Oct 5, 2023

turadg Oct 5, 2023

Chris-Hibbert Oct 5, 2023

turadg Oct 5, 2023

Chris-Hibbert Oct 5, 2023

dckc Oct 5, 2023

FUDCo Oct 5, 2023

dckc Oct 5, 2023

Chris-Hibbert Oct 5, 2023

dckc commented Oct 5, 2023

Chris-Hibbert commented Oct 5, 2023

dckc commented Oct 5, 2023 •

edited

Loading

turadg left a comment

turadg Oct 5, 2023

turadg Oct 5, 2023

Chris-Hibbert Oct 5, 2023

Chris-Hibbert Oct 5, 2023

turadg Oct 5, 2023

turadg Oct 5, 2023

	finishPromiseKit.resolve(true);
	finishPromiseKit.resolve(undefined);

	const test = anyTest as TestFn<LiquidationTestContext>;
	const test = anyTest as TestFn<LiquidationTestContext & { proposal: Awaited<ReturnType<typeof buildProposal>> }>;

refactor: delay addition of issuer to auctioneer until a good time #8398

refactor: delay addition of issuer to auctioneer until a good time #8398

Conversation

Chris-Hibbert commented Sep 27, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Chris-Hibbert commented Sep 28, 2023 • edited Loading

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Oct 5, 2023

Chris-Hibbert commented Oct 5, 2023

dckc commented Oct 5, 2023 • edited Loading

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented Sep 27, 2023 •

edited

Loading

Chris-Hibbert commented Sep 28, 2023 •

edited

Loading

dckc commented Oct 5, 2023 •

edited

Loading